-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix bump phonopy #1006
Fix bump phonopy #1006
Conversation
Hi @JaGeo , I had a look at the phonopy source code changes between v2.28 and v.2.27, I could not really pinpoint what is causing the issue with different values of the average Gruneisen parameter here. I guess it has something to do with following PR. But am not sure about it |
How large are the changes? Could you give more details? Thanks! |
The average Gruneisen value now comes out to be |
Yeah, I think we need a bit more analysis here. This is quite a large change |
The latest version of phonopy causes phonon workflow test to also fail (cause of failure seems to originate from force constants computation default being changed) |
I would need more details to act. E.g., How large is the change? Why does this only affect the Grüneisen parameter workflow? What is different? We can do this privately if you prefer. |
As I have mentioned above, now with latest version phonon workflow test is also affected. I will try to share the details in next days |
@naik-aakash i see! Thanks! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1006 +/- ##
==========================================
+ Coverage 72.82% 76.63% +3.81%
==========================================
Files 187 187
Lines 13637 13627 -10
Branches 1370 1372 +2
==========================================
+ Hits 9931 10443 +512
+ Misses 3161 2626 -535
- Partials 545 558 +13
|
@naik-aakash one last question: do you need all test files still or can one set be removed? |
Hi @JaGeo, Thanks for spotting this. I Have now removed the test files that are no longer used |
@naik-aakash thanks. This looks good. I will merge. |
For everyone else seeing this and wondering why we are changing tests: By accident, one of the phonon tests used wrong test files. This only appeared as a problem when phonopy added more strict handling of the forces. The Grüneisen parameter computation was also affected by a phonopy change. However, as this mostly affected an unrealiable value at gamma, we simply remove it from the mesh computation. We should have likely done this already before. |
* bump phonopy version * Update pyproject.toml * replace bs plotter with pymatgen implementation > reduce code repetition * replace bs plotter with pymatgen implementation > reduce code repetition * switch off grunesien computation at gamma (unstable) * fix assertions to correct expected derived properties * fix failing test (use correct reference data) * remove non used test_data, rename Si_phonons_4 > Si_phonons_3 * point to renamed ref test_data --------- Co-authored-by: J. George <[email protected]>
Changes
Todo